20260720-linuxkm-CMAC-native#10980
Conversation
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10980
Scan targets checked: linuxkm-bugs, linuxkm-src, wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
Retest this please. |
3e6a2f1 to
d4c3410
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10980
Scan targets checked: linuxkm-bugs, linuxkm-src, wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 2 posted, 2 skipped
Posted findings
- [Medium] enabled_fips declaration/use guard mismatch can break -Werror kernel builds —
linuxkm/lkcapi_glue.c:227 - [Medium] configure.ac lean-aesgcm-dev SHA512 logic inverted from disable to force-enable —
configure.ac:7377-7379
Skipped findings
- [Medium] km_hmac_import() calls allocating wc_HmacCopy() while holding spinlock (sleep-in-atomic)
- [Low] cpuid_flag: reg[num] = 0 is dead code (overwritten by cpuid())
Review generated by Skoll via Claude/Codex
…ment linuxkm cmac(aes) glue.
…leaks: * separate WC_LINUXKM_SHA_IMPLEMENT() into WC_LINUXKM_SHA1_IMPLEMENT() (no fixes needed) and WC_LINUXKM_SHA2_IMPLEMENT() (with associated new helpers WC_LINUXKM_SHA2_FREE_W(), WC_LINUXKM_SHA2_DECL_W(), WC_LINUXKM_SHA2_PUSH_W(), and WC_LINUXKM_SHA2_POP_W(), that move .W to a stack buffer). * Reimplement SHA-2 one-shot digest callback to use only direct wolfCrypt calls rather than proxy to other callbacks.
…ecks on kernel struct wolfSSL_Mutex in WOLFSSL_LINUXKM_VERBOSE_DEBUG builds.
…TfmCtx, adding km_sha3_init_tfm(), km_sha3_exit_tfm(), km_sha3_alloc_tstate(), and km_sha3_free_tstate(), implementing garbage collection of abandoned shash_desc objects at .exit_tfm.
…ment km_sha3_export() and per-alg _import() methods, and km_sha3_test_export_import().
* add to struct km_sha_hmac_pstate members desc_list_lock, desc_list, export_list, and export_list_len; * add struct km_sha_hmac_export_state; * add km_hmac_alloc_tstate(), km_hmac_export(), km_hmac_import(), km_hmac_test_export_import(); * and update km_hmac_*() and WC_LINUXKM_HMAC_IMPLEMENT(() to implement.
… conjunctions to fix constant-folding-provoked compiler warnings around fips_enabled; for clarity, rename internal macros WC_LINUXKM_HAVE_SELFTEST and WC_LINUXKM_HAVE_SELFTEST_FULL to WC_LINUX_CONFIG_SELFTESTS and WC_LINUX_CONFIG_SELFTESTS_FULL respectively.
…hten up SHA-3 allocations, avoiding frivolous struct km_sha_state (with inline wc_Sha512) for SHA-3.
* Eliminate pointer from export blob, replacing it with a TFM-specific random cookie and per-desc serial ID. * Fixes kernel address disclosure risk, handle forgery risk, cross-TFM confusion risk, and heap pointer reuse risk.
…sl/wolfcrypt/settings.h: * Revert earlier changes adding WC_SHA256_W_SIZE and WC_SHA512_W_SIZE. * Add WC_SHA2_NO_SMALL_STACK to allow the W work buffer to move back onto the stack. * In wolfssl/wolfcrypt/settings.h, define WC_SHA2_NO_SMALL_STACK by default when WOLFSSL_KERNEL_MODE, and add a clause to #undef WOLFSSL_SMALL_STACK when building the SHA-2 implementations. linuxkm/lkcapi_sha_glue.c: * Refactor out superfluous struct km_sha_state. * Clean up some macro dynamics. * Add static asserts on HASH_MAX_STATESIZE to WC_LINUXKM_SHA1_IMPLEMENT() and WC_LINUXKM_SHA2_IMPLEMENT().
…ntegrity check and DRBG.
… SEGV from gcc misoptimization.
… gating on its declaration (add dependency on defined(CONFIG_CRYPTO_FIPS)); use FIPS_NOT_ALLOWED_E rather than NOT_COMPILED_IN to signal FIPS-forbidden keysizes from linuxkm_test_ecdh_nist_driver() and linuxkm_test_ecdsa_nist_driver() to REGISTER_ALG_OPTIONAL().
d4c3410 to
5d377ee
Compare
|
retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 47 total — 6 posted, 41 skipped
Posted findings
- [High] CMAC transient-copy design breaks under WC_DEBUG_CIPHER_LIFECYCLE: dangling/double-free of Aes.CipherLifecycleTag —
linuxkm/lkcapi_aes_glue.c:5168 - [High] configure.ac lean-aesgcm SHA-512 clause is logically inverted, force-ENABLING SHA-512/SHA-384 in the lean FIPS profile —
configure.ac:7377-7379 - [High] SHA-3 and CMAC shash_algs adopt .init_tfm/.exit_tfm without extending the >= 5.6 kernel-version guard —
linuxkm/lkcapi_sha_glue.c:1177 - [Medium] km_hmac_export reads snapshot->desc_id after releasing the list lock (use-after-free window) —
linuxkm/lkcapi_sha_glue.c:1630-1648 - [Medium] wc_UnLockMutex returns -1 without unlocking on magic mismatch, leaving the spinlock held with IRQs disabled —
linuxkm/linuxkm_wc_port.h:2036-2045 - [Medium] wc_AesGcmInit() WC_C_DYNAMIC_FALLBACK permanently clears aes->use_aesni instead of scoping the fallback to the call —
wolfcrypt/src/aes.c:13993-14008
Skipped findings
- [Medium] km_hmac_import and SHA-3 .init/.import orphan the desc's previous working node onto an uncapped list
- [Medium] km_hmac_import performs wc_HmacCopy (which may allocate) while holding the IRQ-disabling spinlock
- [Medium] WC_SHA256_W_SIZE / WC_SHA512_W_SIZE duplicate private allocation sizes from sha256.c / sha512.c
- [Medium] WC_LINUXKM_SHA2_FREE_W frees an XMALLOC'd buffer with raw free()
- [Medium] The entire SHA-2 stack-W path is compiled out in default kernel builds and therefore ships untested
- [Medium] HMAC .export handle is not self-contained state and is valid only within one tfm and one eviction window
- [Medium] km_hmac_export() reads the snapshot node after publishing it to export_list and dropping the lock (use-after-free read)
- [Medium] New export/import self-tests allocate their tfm by cra_name and assert wolfSSL-private statesize, breaking registration under LINUXKM_LKCAPI_PRIORITY_ALLOW_MASKING
- [Medium] km_hmac_import() orphans a working node per abandoned desc, giving unbounded kernel memory growth on the AF_ALG accept()/close() path
- [Medium] AES-CMAC transient-copy design use-after-frees the Aes CipherLifecycleTag under WC_DEBUG_CIPHER_LIFECYCLE
- [Medium] km_AesCmacSetKey() frees the tfm-owned pristine Cmac that concurrent descs dereference
- [Medium] km_hmac_export() reads snapshot->desc_id after dropping desc_list_lock, racing eviction/free
- [Medium] HMAC export-snapshot ring is sized by CPU count, not in-flight accepts, causing spurious -EINVAL under concurrent AF_ALG accept()
- [Low] SHA-2 one-shot .digest still heap-allocates W, contradicting the stated no-heap-after-init design
- [Low] types.h: undocumented !defined(GNUC) on the C23 static_assert branch changes which implementation GCC/clang select
- [Low] configure.ac 'cmac(aes)' defines HAVE_AES_ECB (unused) instead of WOLFSSL_AES_DIRECT (required by the glue gate)
- [Low] configure.ac lean-aesgcm: SHA-512 force-enable adds -DWOLFSSL_SHA384 while ENABLED_SHA384 stays "no"
- [Low] km_AesCmacFinal/Finup bypass km_AesCmacDispose and bare-free the Cmac
- [Low] SHA-3 one-shot digest places a ~480-byte struct on the kernel stack
- [Low] km_AesCmacSetKey does not fail closed on the key-length rejection path
- [Low] CMAC transient copy does not defensively NULL aes.streamData, unlike km_AesGet() in the same file
- [Low] cpuid.c: replacing XMEMSET of the register array with a single-element zero is unrelated to this PR
- [Low] Bare scope blocks introduced in km_AesCmacFinal / km_AesCmacFinup
- [Low] KMAC-256 FIPS path drops the zero-length-key check instead of asserting KMAC_MIN_KEYLEN_E
- [Low] SHA-3 .init and .import orphan any prior state node onto an unbounded per-TFM desc_list
- [Low] Bounded HMAC export ring makes a previously valid state blob permanently unimportable, so concurrent AF_ALG accept() can fail spuriously
- [Low] km_AesCmacSetKey() does not fail closed on the invalid-key-length path, contradicting its own rekey invariant
- [Low] SHA-3 export blob carries no algorithm/variant tag, permitting cross-variant state import
- [Low] cpuid.c XMEMSET removal not applied to cpuid_is_intel()/cpuid_is_amd(), which run first on the same path
- [Low] km_hmac_import() performs the deep wc_HmacCopy() while holding the IRQ-disabling spinlock
- [Low] wc_FreeMutex() now invalidates the lock, and wc_UnLockMutex() can bail out while still holding the spinlock
- [Low] km_hmac_free_tstate()/km_sha3_free_tstate() silently skip cleanup when the list lock cannot be taken
- [Info] linuxkm_test_aescmac ForceZeros an uninitialized stack struct on early-exit paths
- [Info] wc_FreeMutex uses memset (not XMEMSET/ForceZero) and keeps a now-vestigial (void)m
- [Info] WC_LINUXKM_SPINLOCK_MAGIC is an unexplained decimal literal
- [Info] list_last_entry() without a list_empty() guard, protected only by an assert that can be silently skipped
- [Info] Empty #if/#endif block left behind in the selftest-macro rename
- [Info] settings.h #undefs WOLFSSL_SMALL_STACK before headers are parsed, changing struct layouts inside the sha256.c/sha512.c translation units
- [Info] WC_LINUXKM_SHA2_FREE_W() releases an XMALLOC'd buffer with raw free()
- [Info] WC_LINUXKM_SHA2_PUSH_W/POP_W generate bare brace-scope blocks in every SHA-2 shash entry point
- [Info] SHA-3 .digest places a ~448-byte struct on the stack, contradicting the HMAC digest's stated rationale
Review generated by Skoll via Claude/Codex
| if (! cmac) | ||
| return -ENOMEM; | ||
|
|
||
| XMEMCPY(cmac, t_ctx->pristine, sizeof(*cmac)); |
There was a problem hiding this comment.
🟠 [High] CMAC transient-copy design breaks under WC_DEBUG_CIPHER_LIFECYCLE: dangling/double-free of Aes.CipherLifecycleTag
🚫 BLOCK bug
The new CMAC glue documents an invariant — "the embedded Aes owns no heap allocations in module configurations: wc_AesFree()'s only mainstream XFREE() target is the GCM-streaming streamData buffer" — and enforces it with a single #error for WOLF_CRYPTO_CB_FREE. That invariant is false under WC_DEBUG_CIPHER_LIFECYCLE: wolfssl/wolfcrypt/aes.h:444 adds void *CipherLifecycleTag, wolfcrypt/src/aes.c:15475 XMALLOCs it in wc_AesInit(), and wolfcrypt/src/aes.c:15553 XFREEs it (and NULLs it) in wc_AesFree(). km_AesCmacMaterialize() struct-copies the tfm-owned pristine Cmac (including that pointer), and every km_AesCmacUpdate()/Final()/Finup() then destroys the copy via wc_CmacFree()/wc_CmacFinal() -> wc_AesFree(), which frees the tag while t_ctx->pristine->aes.CipherLifecycleTag still points at it. The next materialize copies the dangling pointer and wc_debug_CipherLifecycleCheck() reads freed kernel memory. WOLF_CRYPTO_CB without _FREE is a second unguarded case: Cmac.devCtx is a per-instance pointer that would likewise ride every copy.
Suggestion:
| XMEMCPY(cmac, t_ctx->pristine, sizeof(*cmac)); | |
| #if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) | |
| #error LINUXKM_LKCAPI_REGISTER_AESCMAC is incompatible with WOLF_CRYPTO_CB_FREE. | |
| #endif | |
| #ifdef WC_DEBUG_CIPHER_LIFECYCLE | |
| #error LINUXKM_LKCAPI_REGISTER_AESCMAC is incompatible with WC_DEBUG_CIPHER_LIFECYCLE. | |
| #endif |
| \ | ||
| static struct shash_alg name ## _alg = \ | ||
| { \ | ||
| .init_tfm = km_sha3_init_tfm, \ |
There was a problem hiding this comment.
🟠 [High] SHA-3 and CMAC shash_algs adopt .init_tfm/.exit_tfm without extending the >= 5.6 kernel-version guard
💡 SUGGEST
struct shash_alg's init_tfm/exit_tfm members were added in Linux 5.6. The file already encodes that fact in an explicit guard at lines 409-420 -- #error LINUXKM_LKCAPI_REGISTER for HMACs is supported only on Linux kernel versions >= 5.6.0. -- whose condition enumerates only the eight *_HMAC register macros, because HMAC was the only user of those callbacks. This PR adds .init_tfm = km_sha3_init_tfm and .exit_tfm = km_sha3_exit_tfm to the SHA-3 shash_alg (lines 1177/1188) and .exit_tfm = km_AesCmacExitTfm to cmacAesAlg (lkcapi_aes_glue.c:5350), but does not extend the guard's condition to cover LINUXKM_LKCAPI_REGISTER_SHA3_224/256/384/512 or LINUXKM_LKCAPI_REGISTER_AESCMAC. The module advertises support back to 5.4 (lkcapi_glue.c:30-32: #error LINUXKM_LKCAPI_REGISTER is supported only on Linux kernel versions >= 5.4.0.). Trigger: build against a 5.4 or 5.5 kernel with --enable-linuxkm-lkcapi-register=sha3 (or all, or cmac(aes)). Instead of the intended clear #error, the build fails with an opaque 'struct shash_alg has no member named init_tfm' from the macro expansion, or -- worse if a distro backport added only one of the two members -- compiles with the callback silently dropped, which for SHA-3 means desc_list_lock is never initialized and km_sha3_alloc_tstate() locks an uninitialized spinlock on the first digest.
Recommendation: Extend the guard's condition to every alg that now installs an init_tfm/exit_tfm callback, and retitle the message:
#if (LINUX_VERSION_CODE < KERNEL_VERSION(5, 6, 0)) &&
(defined(LINUXKM_LKCAPI_REGISTER_SHA1_HMAC) ||
... existing HMAC entries ...
defined(LINUXKM_LKCAPI_REGISTER_SHA3_224) ||
defined(LINUXKM_LKCAPI_REGISTER_SHA3_256) ||
defined(LINUXKM_LKCAPI_REGISTER_SHA3_384) ||
defined(LINUXKM_LKCAPI_REGISTER_SHA3_512))
#error LINUXKM_LKCAPI_REGISTER for HMACs and SHA-3 is supported only on Linux kernel versions >= 5.6.0.
#endif
and add the matching guard for LINUXKM_LKCAPI_REGISTER_AESCMAC in lkcapi_aes_glue.c.
| snapshot->desc_id = p_ctx->cur_desc_id++; | ||
| /* list_add() prepends, so the tail from list_last_entry() is the oldest. */ | ||
| list_add(&snapshot->desc_ent, &p_ctx->export_list); | ||
| p_ctx->export_list_len++; |
There was a problem hiding this comment.
🟡 [Medium] km_hmac_export reads snapshot->desc_id after releasing the list lock (use-after-free window)
🚫 BLOCK bug
The function's own design note says unlinking under the lock is what makes the ring safe: "the evicted node is now unlinked and unreachable ... so free it outside the lock." But after wc_UnLockMutex() the code dereferences snapshot again to fill in the blob (blob->desc_id = snapshot->desc_id;). Once the lock is dropped, a concurrent km_hmac_export() on the same tfm can evict this very node (it is the list head, becoming the tail after WC_LINUXKM_HMAC_EXPORT_LIST_MAX further exports) and free() it, so the read races a free. Every other post-unlock dereference in the function is on the deliberately-unlinked evicted node; this one is on a still-published node.
Suggestion:
| p_ctx->export_list_len++; | |
| word64 new_desc_id; | |
| ... | |
| new_desc_id = snapshot->desc_id = p_ctx->cur_desc_id++; | |
| list_add(&snapshot->desc_ent, &p_ctx->export_list); | |
| p_ctx->export_list_len++; | |
| (void)wc_UnLockMutex(&p_ctx->desc_list_lock); | |
| ... | |
| blob->desc_id = new_desc_id; |
Implement glue logic for Linux kernel registration of native
cmac(aes)implementation, and fix gaps in several otherstruct shash_algglue implementations:linuxkm/lkcapi_aes_glue.c,linuxkm/lkcapi_glue.c,configure.ac: implement linuxkmcmac(aes)glue.linuxkm/lkcapi_sha_glue.c: fixsha.Wlifecycle management to prevent leaks:separate
WC_LINUXKM_SHA_IMPLEMENT()intoWC_LINUXKM_SHA1_IMPLEMENT()(no fixesneeded) and
WC_LINUXKM_SHA2_IMPLEMENT()(with associated new helpersWC_LINUXKM_SHA2_FREE_W(),WC_LINUXKM_SHA2_DECL_W(),WC_LINUXKM_SHA2_PUSH_W(),and
WC_LINUXKM_SHA2_POP_W(), that move.Wto a stack buffer).Reimplement SHA-2 one-shot digest callback to use only direct wolfCrypt calls
rather than proxy to other callbacks.
linuxkm/linuxkm_wc_port.h,linuxkm/module_hooks.c: implementmagicchecks on kernelstruct wolfSSL_MutexinWOLFSSL_LINUXKM_VERBOSE_DEBUGbuilds.linuxkm/lkcapi_sha_glue.c: refactor SHA-3 state aroundstruct km_Sha3TfmCtx,adding
km_sha3_init_tfm(),km_sha3_exit_tfm(),km_sha3_alloc_tstate(), andkm_sha3_free_tstate(), implementing garbage collection of abandonedshash_descobjects at
.exit_tfm.linuxkm/lkcapi_sha_glue.c: addstruct km_sha3_export_state, and implementkm_sha3_export()and per-alg_import()methods, andkm_sha3_test_export_import().linuxkm/lkcapi_sha_glue.c: fix export/import for HMAC:struct km_sha_hmac_pstatemembersdesc_list_lock,desc_list,export_list, andexport_list_len;struct km_sha_hmac_export_state;km_hmac_alloc_tstate(),km_hmac_export(),km_hmac_import(),km_hmac_test_export_import();km_hmac_*()andWC_LINUXKM_HMAC_IMPLEMENT() to implement.test/:
NO_DH;test_wc_ed448_import_public()andtest_wc_Ed448DecisionCoverage()for FIPS v6;tests/api/test_sha3.cfor KMAC keysize in FIPS builds.tested with